CCXDEV-14850: align insights DataGather with config #2248

opokornyy · 2025-03-26T12:30:12Z

This PR should align the configuration options in DataGather with the InsightsDataGather

openshift-ci-robot · 2025-03-26T12:30:16Z

@opokornyy: This pull request references CCXDEV-14850 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.19.0" version, but no target version was set.

In response to this:

This PR should align the configuration options in DataGather with the InsightsDataGather

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-03-26T12:30:18Z

Hello @opokornyy! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

opokornyy · 2025-03-27T10:54:34Z

/cc @ncaak @tremes

everettraven

I believe most of this API I've seen before and looks good. I've left comments on stuff that seemed new to me

insights/v1alpha2/types_insights.go

everettraven · 2025-04-04T11:26:45Z

Just leaving a general note that some of the changes I've requested are a deviation from the v1alpha1 API that I think would be inherently breaking (i.e removing fields, etc.). I'm still learning what this means for alpha level APIs that are only in TPNU clusters, but I suspect we would likely want to go back and bring the v1alpha1 APIs into alignment (or drop them?).

@JoelSpeed what is the correct approach to follow here?

insights/v1alpha2/types_insights.go

JoelSpeed · 2025-04-08T12:19:57Z

Since we aren't going to allow clusters to upgrade between v1alpha1 and v1alpha2, we don't need to make changes in the alpha 1 or care about conversion webhooks, so we can make breaking changes here

everettraven

A couple more comments plus my still outstanding comment on the TotalRisk field.

Other than that, I think this looks good.

@JoelSpeed will need to make a pass on it prior to it being able to merge though

insights/v1alpha2/types_insights.go

everettraven

A couple more comments. Other than these, I think it looks good.

tagging in @JoelSpeed for a review.

insights/v1alpha2/types_insights.go

insights/v1alpha2/Makefile

insights/v1alpha2/doc.go

insights/v1alpha2/types_insights.go

everettraven

I don't have any additional comments on this PR. Looks like the only outstanding comment is from @JoelSpeed related to the None gathering mode.

This commit introduces the v1alpha2 version of the DataGather CRD for Insights. Signed-off-by: Ondrej Pokorny <[email protected]>

JoelSpeed

Couple of small bits to fix around wording, please add those bits about starting and ending to both the godoc and the CEL error message.

And fix the validation on resource from 1123 to 1035 label as there is a subtle difference

JoelSpeed · 2025-05-08T15:46:31Z

insights/v1alpha2/types_insights.go

+const (
+	TotalRiskLow       TotalRisk = "Low"
+	TotalRiskModerate  TotalRisk = "Moderate"
+	TotalRiskImportant TotalRisk = "Important"
+	TotalRiskCritical  TotalRisk = "Critical"
+)


Nit: Why Low/Moderate/Important/Critical? I was expecting High to follow Moderate

I don't have a strong opinion, but I'll ask @tremes whether this naming was introduced in IO and if it's okay to change it

This reflects the severity levels defined for the Insights recommendation so it's not easy to change.

Ack, that's ok then, just felt like it didn't fit, but if it's already shipped it's already shipped

insights/v1alpha2/types_insights.go

This commit aligns the configuration options of the DataGather CRD with those of InsightsDataGather, making it easier to use both CRDs. Signed-off-by: Ondrej Pokorny <[email protected]>

JoelSpeed · 2025-05-13T09:23:38Z

/lgtm

openshift-ci · 2025-05-13T09:24:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, JoelSpeed, opokornyy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2025-05-13T13:25:29Z

@opokornyy: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-bot · 2025-05-14T08:25:20Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-config-api
This PR has been included in build ose-cluster-config-api-container-v4.20.0-202505140744.p0.g9052dea.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 26, 2025

openshift-ci bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 26, 2025

openshift-ci bot requested review from everettraven and JoelSpeed March 26, 2025 12:31

opokornyy force-pushed the CCXDEV-15040-insights-v1alpha2 branch 2 times, most recently from af4174b to 4bada1b Compare March 27, 2025 08:07

openshift-ci bot requested review from ncaak and tremes March 27, 2025 10:54

everettraven reviewed Apr 1, 2025

View reviewed changes

opokornyy force-pushed the CCXDEV-15040-insights-v1alpha2 branch 2 times, most recently from 4725f08 to 866a461 Compare April 4, 2025 07:33

tremes reviewed Apr 4, 2025

View reviewed changes

insights/v1alpha2/types_insights.go Outdated Show resolved Hide resolved

everettraven reviewed Apr 7, 2025

View reviewed changes

insights/v1alpha2/types_insights.go Outdated Show resolved Hide resolved

everettraven reviewed Apr 7, 2025

View reviewed changes

insights/v1alpha2/types_insights.go Outdated Show resolved Hide resolved

opokornyy force-pushed the CCXDEV-15040-insights-v1alpha2 branch 3 times, most recently from 811a9a2 to d548836 Compare April 10, 2025 13:19

everettraven reviewed Apr 10, 2025

View reviewed changes

insights/v1alpha2/types_insights.go Outdated Show resolved Hide resolved

insights/v1alpha2/types_insights.go Outdated Show resolved Hide resolved

insights/v1alpha2/types_insights.go Outdated Show resolved Hide resolved

opokornyy force-pushed the CCXDEV-15040-insights-v1alpha2 branch from d548836 to 0fa9cfb Compare April 11, 2025 11:44

everettraven reviewed Apr 14, 2025

View reviewed changes

insights/v1alpha2/types_insights.go Outdated Show resolved Hide resolved

insights/v1alpha2/types_insights.go Outdated Show resolved Hide resolved

insights/v1alpha2/types_insights.go Outdated Show resolved Hide resolved

opokornyy force-pushed the CCXDEV-15040-insights-v1alpha2 branch from 0fa9cfb to 882784e Compare April 23, 2025 12:11

JoelSpeed reviewed Apr 24, 2025

View reviewed changes

opokornyy force-pushed the CCXDEV-15040-insights-v1alpha2 branch 3 times, most recently from bb217af to 522fbdc Compare April 29, 2025 11:18

opokornyy requested a review from JoelSpeed April 30, 2025 11:18

everettraven approved these changes May 1, 2025

View reviewed changes

opokornyy force-pushed the CCXDEV-15040-insights-v1alpha2 branch from 522fbdc to c73b718 Compare May 6, 2025 06:54

feat: introduce v1alpha2 for Insights DataGather

4d05e47

This commit introduces the v1alpha2 version of the DataGather CRD for Insights. Signed-off-by: Ondrej Pokorny <[email protected]>

opokornyy force-pushed the CCXDEV-15040-insights-v1alpha2 branch 2 times, most recently from b32ffb8 to 1806f37 Compare May 6, 2025 08:03

JoelSpeed reviewed May 8, 2025

View reviewed changes

feat: align DataGather with InsightsDataGather

7f939ff

This commit aligns the configuration options of the DataGather CRD with those of InsightsDataGather, making it easier to use both CRDs. Signed-off-by: Ondrej Pokorny <[email protected]>

opokornyy force-pushed the CCXDEV-15040-insights-v1alpha2 branch from 1806f37 to 7f939ff Compare May 9, 2025 11:54

opokornyy requested a review from JoelSpeed May 13, 2025 06:10

openshift-ci bot assigned JoelSpeed May 13, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2025

openshift-merge-bot bot merged commit 9052dea into openshift:master May 13, 2025
12 checks passed

opokornyy mentioned this pull request May 13, 2025

bump(api): update api to get insights v1alpha2 openshift/client-go#322

Merged

CCXDEV-14850: align insights DataGather with config #2248

CCXDEV-14850: align insights DataGather with config #2248

Uh oh!

Conversation

opokornyy commented Mar 26, 2025

Uh oh!

openshift-ci-robot commented Mar 26, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Mar 26, 2025

Uh oh!

opokornyy commented Mar 27, 2025

Uh oh!

everettraven left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

everettraven commented Apr 4, 2025

Uh oh!

Uh oh!

Uh oh!

JoelSpeed commented Apr 8, 2025

Uh oh!

everettraven left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

everettraven left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

everettraven left a comment

Choose a reason for hiding this comment

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

JoelSpeed May 8, 2025

Choose a reason for hiding this comment

Uh oh!

opokornyy May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tremes May 13, 2025

Choose a reason for hiding this comment

Uh oh!

JoelSpeed May 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoelSpeed commented May 13, 2025

Uh oh!

openshift-ci-robot commented Mar 26, 2025 •

edited by openshift-ci bot

Loading

opokornyy May 9, 2025 •

edited

Loading